Skip to content

Modify client-side behavior#4563

Merged
mohamed-essam merged 5 commits intofeat/auto-upgradefrom
auto-upgrade-mod
Oct 12, 2025
Merged

Modify client-side behavior#4563
mohamed-essam merged 5 commits intofeat/auto-upgradefrom
auto-upgrade-mod

Conversation

@mohamed-essam
Copy link
Copy Markdown
Contributor

@mohamed-essam mohamed-essam commented Oct 1, 2025 •
edited
Loading

Describe your changes

  • Add an option for only updating on startup (To be implemented on management-side, defaults to true)
    • Change AutoUpdateVersion to a struct for adding additional values
    • Add AlwaysUpdate to AutoUpdate struct, when true, the client will update as soon as possible regardless of time of connection, when false, the client will only update within the first minute of Engine startup
  • Add timeout for updating on startup
    • Agreed to be 1 Minute since startup inclusive of all operations (Fetching latest version, Downloading installation file, and Installation itself).
  • Add progress window for auto-updates
    • Add new flag for netbird-ui to show Updating... screen, no actual progress is shown, just that it's updating.
    • Add new handler for an event for showing update window.
    • Send new event to netbird-ui as soon as Update is in progress (right before installation file starts download), the window is hidden on Error, or on installation success (handled by netbird-ui being restarted by installation method).
  • Persist state for auto-update to show after-update notification.
    • Add new State for AutoUpdate with currentVersion and targetVersion
    • Verify state on UpdateManager initialization to check if last autoUpdate operation was successful to show a notification with success and clear the state.

Manual testing done

  • New window tested in isolation by running netbird-ui.exe --update.
  • Update event sequence tested by injecting an Error delayed by 10s into updateFunc.
  • Timeout tested by injecting 100s delay during file download.
  • After-update notification tested by commenting actual installer running code, then stopping the service and ui manually, then replacing the binaries with a binary running the target latest version (0.58.2 at time of testing), and checking the state.json file.
  • Actual update tested by running the current PR code as-is.

Issue ticket number and link

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

https://github.com/netbirdio/docs/pull/__

@mohamed-essam mohamed-essam force-pushed the auto-upgrade-mod branch 2 times, most recently from e891783 to f42e189 Compare October 1, 2025 14:29
@mohamed-essam mohamed-essam marked this pull request as ready for review October 1, 2025 17:48
pappz
pappz requested changes Oct 7, 2025
View reviewed changes
Comment thread client/internal/updatemanager/manager.go Outdated
import (
"context"
"fmt"
"github.com/netbirdio/netbird/client/internal/statemanager"
Copy link
Copy Markdown
Collaborator

@pappz pappz Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep the order of the imported packages as usual

Copy link
Copy Markdown
Collaborator

@pappz pappz Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And everywhere else.

pappz
pappz requested changes Oct 7, 2025
View reviewed changes
Comment thread client/internal/connect.go Outdated
c.engineMutex.Lock()
c.engine = NewEngine(engineCtx, cancel, signalClient, mgmClient, relayManager, engineConfig, mobileDependency, c.statusRecorder, checks)
if loginResp.PeerConfig != nil && loginResp.PeerConfig.AutoUpdate != nil {
if c.engine.updateManager == nil && loginResp.PeerConfig.AutoUpdate.Version != "disabled" {
Copy link
Copy Markdown
Collaborator

@pappz pappz Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this string come from? It should be const

Comment thread client/internal/connect.go Outdated
c.engineMutex.Lock()
c.engine = NewEngine(engineCtx, cancel, signalClient, mgmClient, relayManager, engineConfig, mobileDependency, c.statusRecorder, checks)
if loginResp.PeerConfig != nil && loginResp.PeerConfig.AutoUpdate != nil {
if c.engine.updateManager == nil && loginResp.PeerConfig.AutoUpdate.Version != "disabled" {
Copy link
Copy Markdown
Collaborator

@pappz pappz Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code duplication with the engine. Move this logic into a function

@pappz
Copy link
Copy Markdown
Collaborator

pappz commented Oct 7, 2025

Please fix the build issues

pappz
pappz requested changes Oct 8, 2025
View reviewed changes
Comment thread client/internal/updatemanager/manager.go
return u
}

func (u *UpdateManager) StartWithTimeout(ctx context.Context, timeout time.Duration) {
Copy link
Copy Markdown
Collaborator

@pappz pappz Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen to the fetcher after a timeout? It seems like it will never be freed

Copy link
Copy Markdown
Contributor Author

@mohamed-essam mohamed-essam Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since timeout will cancel the context, onContextCancel will handle fetcher cleanup

Comment thread client/internal/updatemanager/manager.go
u.wg.Wait()
}

func (u *UpdateManager) onContextCancel() {
Copy link
Copy Markdown
Collaborator

@pappz pappz Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You clean up updateManager with Stop and also with context cancellation. This means we have a race condition in the onContextCancel and Stop() functions.

Copy link
Copy Markdown
Contributor Author

@mohamed-essam mohamed-essam Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the locking mechanism from the original PR modifications

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Oct 8, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@mohamed-essam mohamed-essam merged commit 5556ff3 into feat/auto-upgrade Oct 12, 2025
39 checks passed
@mohamed-essam mohamed-essam deleted the auto-upgrade-mod branch October 12, 2025 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pappz pappz pappz requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mohamed-essam @pappz